Thread: Segmentation Fault [ runtime crash ] - I tried....

Error: Segmentation Fault

I recommend running this program for yourself.

Unfortunately the shell gave me no other information about the error, I believe there was a method for getting more, but the last time I tried that with segmentation fault the line numbers given pointed me in the wrong direction, or so it seemed ( found the error in a totally different spot).

This is for an assignment. I was actually given the source code for the function I believe this error is associated as starter code for the assignment, along with other functions like add_group. My job was to change the functions to return dynamically allocated strings instead of ints (part of the assignment anyway).

Thing is, with the function add_xct even when I replace the function with the initial starter code version of it, I get the same segmentation fault error. That is important information, all of the files compile for this assignment.

*****IMPORTANT****

You can test the code for yourself, but take my word for it, when I type the buxfer command: add_xct [arg1] [arg2]

I get the segmentation fault error at runtime and the program crashes.

Check out the call to add_xct in process_args in buxfer.c

Check out the function implementation of add_xct in lists.c

Check out the function header of add_xct in lists.h

Buxfer.c source code:

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "lists.h"


#define INPUT_BUFFER_SIZE 256
#define INPUT_ARG_MAX_NUM 5
#define DELIM " \n"




/* A standard template for error messages */
void error(const char *msg) {
    fprintf(stderr, "Error: %s\n", msg);
}


/* 
 * Read and process buxfer commands
 */
int process_args(int cmd_argc, char **cmd_argv, Group **group_list_addr, char *user_name) {
    Group *group_list = *group_list_addr; 
    Group *g;
    char *buffer;


    if (cmd_argc <= 0) {
        return 0;
    } else if (strcmp(cmd_argv[0], "quit") == 0 && cmd_argc == 1) {
        return -1;
        
    } else if (strcmp(cmd_argv[0], "add_group") == 0 && cmd_argc == 2) {
        buffer = add_group(group_list_addr, cmd_argv[1], user_name);         
        printf("%s", buffer);
        
    } else if (strcmp(cmd_argv[0], "list_groups") == 0 && cmd_argc == 1) {
        buffer = list_groups(group_list);        
        printf("%s", buffer);
        
    } else if (strcmp(cmd_argv[0], "add_user") == 0 && cmd_argc == 3) {
        if ((g = find_group(group_list, cmd_argv[1])) == NULL) {
            error("Group does not exist");
        } else {
            buffer = add_user(g, cmd_argv[2]);            
            printf("%s", buffer);
        }
        
    } else if (strcmp(cmd_argv[0], "list_users") == 0 && cmd_argc == 2) {
        if ((g = find_group(group_list, cmd_argv[1])) == NULL) {
            error("Group does not exist");
        } else {
            buffer = list_users(g);            
            printf("%s", buffer);
        }
        
    } else if (strcmp(cmd_argv[0], "user_balance") == 0 && cmd_argc == 3) {
        if ((g = find_group(group_list, cmd_argv[1])) == NULL) {
            error("Group does not exist");
        } else {
            buffer = user_balance(g, cmd_argv[2]);            
            printf("%s", buffer);
        }
        
    } else if (strcmp(cmd_argv[0], "add_xct") == 0 && cmd_argc == 3) {
        if ((g = find_group(group_list, cmd_argv[1])) == NULL) {
            error("Group does not exist");
        } else {
            char *end;
            double amount = strtod(cmd_argv[3], &end);
            if (end == cmd_argv[3]) {
                error("Incorrect number format");
            } else {
                buffer = add_xct(g, cmd_argv[2], amount);
                printf("%s", buffer);
            }
        }


    } else {
        error("Incorrect syntax");
    }
    return 0;
}


int main(int argc, char* argv[]) {


    char input[INPUT_BUFFER_SIZE];
    char *cmd_argv[INPUT_ARG_MAX_NUM];
    char user_name[256];
    strcpy(user_name, "");
    int user_name_len = strlen(user_name);
    int condition = 0;
    int cmd_argc;
    FILE *input_stream;
        
    /* Initialize the list head */
    Group *group_list = NULL;


    /* Batch mode */
    if (argc == 2) {
        input_stream = fopen(argv[1], "r");
        if (input_stream == NULL) {
            error("Error opening file");
            exit(1);
        }
    }
    /* Interactive mode */
    else {
        input_stream = stdin;
    }
    printf("Welcome to Buxfer!\nPlease input your username:\n>");
    
    while (fgets(input, INPUT_BUFFER_SIZE, input_stream) != NULL) {
        /* Echo line if in batch mode */
        if (user_name_len == 0) {
            condition = 1;
        }
        if (argc == 2) {
            printf("%s", input);
        }
        /* Tokenize arguments */
        char *next_token = strtok(input, DELIM);
        cmd_argc = 0;
        while (next_token != NULL) {
            if (cmd_argc >= INPUT_ARG_MAX_NUM - 1) {
                error("Too many arguments!");
                cmd_argc = 0;
                break;
            }
            cmd_argv[cmd_argc] = next_token;
            cmd_argc++;
            next_token = strtok(NULL, DELIM);
        }
        cmd_argv[cmd_argc] = NULL;
        if (condition) {
            if (cmd_argc == 1) {
                strcpy(user_name, input);
                printf("%s", user_name);
                user_name_len = strlen(user_name);
                condition = 0;
                printf("Welcome to Buxfer!\nPlease enter supported buxfer commands:\n>");
            } else {
                printf("Only one word (argument) is required for username.\n");
            }
        } else {
            if (cmd_argc > 0 && process_args(cmd_argc, cmd_argv, &group_list, user_name) == -1) {
                break; /* quit command was entered */
            }
        }
        printf(">");
    }


    /* Close file if in batch mode */
    if (argc == 2) {
        fclose(input_stream);
    }
    return 0;
}

lists.c source code:

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "lists.h"


/* Add a group with name group_name to the group_list referred to by 
* group_list_ptr. The groups are ordered by the time that the group was 
* added to the list with new groups added to the end of the list.
*
* Returns 0 on success and -1 if a group with this name already exists.
*
* (I.e, allocate and initialize a Group struct, and insert it
* into the group_list. Note that the head of the group list might change
* which is why the first argument is a double pointer.) 
*/
char *add_group(Group **group_list_ptr, const char *group_name, char *user_name) {
    
   if (find_group(*group_list_ptr, group_name) == NULL) {
        //malloc space for new group
        Group *newgrp;
        char *groupSucc; 
        if ((groupSucc = malloc(strlen(group_name) + 30)) == NULL) {
            perror("Error allocating space for add_group success message");
            exit(1);
        }
        strcpy(groupSucc, "Group ");
        strcat(groupSucc, group_name);
        strcat(groupSucc, " added\r\n");
        if ((newgrp = malloc(sizeof(Group))) == NULL) {
           perror("Error allocating space for new Group");
           exit(1);
        }
        // set the fields of the new group node
        // first allocate space for the name
        int needed_space = strlen(group_name) + 1;
        if (( newgrp->name = malloc(needed_space)) == NULL) {
           perror("Error allocating space for new Group name");
           exit(1);
        }
        strncpy(newgrp->name, group_name, needed_space);
        newgrp->users = NULL;
        newgrp->xcts = NULL;
        newgrp->next = NULL;
        // Need to insert into the end of the list not the front
        if (*group_list_ptr == NULL) {
          // set new head to this new group -- the list was previously empty
          *group_list_ptr = newgrp;          
          // implement add_user section
          char *add_userMSG = add_user(newgrp, user_name);
          if ((groupSucc = realloc(groupSucc, sizeof(groupSucc) + strlen(add_userMSG) + 10)) == NULL ) {
              perror("Error allocating space for add_group success message");
            exit(1);
          }
          strcat(groupSucc, add_userMSG); 
          strcat(groupSucc, "\n"); 
          return groupSucc;  
        }  else {
        // walk along the list until we find the currently last group 
          Group * current = *group_list_ptr;
          while (current->next != NULL) {
            current = current->next;
          }
          // now current should be the last group 
          current->next = newgrp;
          char *add_userMSG = add_user(newgrp, user_name);
          if ((groupSucc = realloc(groupSucc, sizeof(groupSucc) + strlen(add_userMSG) + 10)) == NULL ) {
              perror("Error allocating space for add_group success message");
            exit(1);
          }
          strcat(groupSucc, add_userMSG); 
          strcat(groupSucc, "\n"); 
          return groupSucc;
        }
    } else {
            // group EXISTS
            char *groupFail;
            if ((groupFail = malloc(strlen(group_name) + 30)) == NULL) {
                perror("Error allocating space for add_group failure message");
                exit(1);
            }
            strcpy(groupFail, "Group ");
            strcat(groupFail, group_name);
            strcat(groupFail, " already exists\r\n");
            return groupFail;
    }
}


/* Print to standard output the names of all groups in group_list, one name
*  per line. Output is in the same order as group_list.
*/
char *list_groups(Group *group_list) {
    Group * current = group_list;
    char *groupList;
    // What if there are no groups, do I have to strcpy an empty string?
    if ((groupList = malloc(sizeof(char))) == NULL) {
        perror("Error allocating space for the string groupList");
        exit(1);
    }    
    while (current != NULL) {
        if ((groupList = realloc(groupList, sizeof(groupList) + strlen(current->name) + 10)) == NULL) {
            perror("Error resising the memory allocated to groupList");
            exit(1);
        }
        strcat(groupList, current->name);
        strcat(groupList, "\r\n");
        current = current->next;
    }    
    return groupList;
}


/* Search the list of groups for a group with matching group_name
* If group_name is not found, return NULL, otherwise return a pointer to the 
* matching group list node.
*/
Group *find_group(Group *group_list, const char *group_name) {
    Group *current = group_list;
    while (current != NULL && (strcmp(current->name, group_name) != 0)) {
        current = current->next;
    }
    return current;
}


/* Add a new user with the specified user name to the specified group. Return zero
* on success and -1 if the group already has a user with that name.
* (allocate and initialize a User data structure and insert it into the
* appropriate group list)
*/
char *add_user(Group *group, const char *user_name) {
    User *this_user = find_prev_user(group, user_name);
    char *userFail;
    char *userSucc;
    int name_len = strlen(user_name);
    if (this_user != NULL) {
        if ((userFail = malloc(strlen(user_name) + 30)) == NULL) {
            perror("Error allocating space for new User name");
            exit(1);
        }strcpy(userFail, user_name);
        strcat(userFail, " is already in the group\r\n");
        return userFail;
    }
    // ok to add a user to this group by this name
    // since users are stored by balance order and the new user has 0 balance
    // he goes first
    User *newuser;
    if ((newuser = malloc(sizeof(User))) == NULL) {
        perror("Error allocating space for new User");
        exit(1);
    }
    // set the fields of the new user node
    // first allocate space for the name   
    if ((newuser->name = malloc(name_len + 1)) == NULL) {
        perror("Error allocating space for new User name");
        exit(1);
    }
    strncpy(newuser->name, user_name, name_len + 1);    
    newuser->balance = 0.0;
    // insert this user at the front of the list
    newuser->next = group->users;
    group->users = newuser;
    if ((userSucc = malloc(strlen(user_name) + 35)) == NULL) {
            perror("Error allocating space for new User name");
            exit(1);
    }strcpy(userSucc, user_name);
    strcat(userSucc, " successfully added to group\n");
    return userSucc;
}


/* Print to standard output the names and balances of all the users in group,
* one per line, and in the order that users are stored in the list, namely 
* lowest payer first.
*/
char *list_users(Group *group) {
    User *current_user = group->users;
    char *userBalanceList;
    if ((userBalanceList = malloc(sizeof(char))) == NULL) {
        perror("Error allocating space for the string userBalanceList");
        exit(1);
    }    
    while (current_user != NULL) {
        //for the if statement below do not forget to wrap current_user->balance in a string
        int balance_len = snprintf(NULL, 0, "%.2f", current_user->balance);
        char balance_str[balance_len + 1];
        snprintf(balance_str, balance_len + 1, "%.2f",current_user->balance);
        if ((userBalanceList = realloc(userBalanceList, sizeof(userBalanceList) + strlen(current_user->name) + balance_len + 10)) == NULL) {
            perror("Error resising the memory allocated to userBalanceList");
            exit(1);
        }
        //wrap current_user->balance in a string and simplify expression
        strcat(userBalanceList, current_user->name);
        strcat(userBalanceList, " ");
        strcat(userBalanceList, balance_str);
        strcat(userBalanceList, "\r\n");
        current_user = current_user->next;
    }
    return userBalanceList;
    
}
/* Print to standard output the balance of the specified user. Return 0
* on success, or -1 if the user with the given name is not in the group.
*/
char *user_balance(Group *group, const char *user_name) {
    User * prev_user = find_prev_user(group, user_name);
    char *userFail;
    char *userB;
    if (prev_user == NULL) { 
        //wrap balance
        if ((userFail = malloc(strlen(user_name) + 35)) == NULL) {
            perror("Error resising the memory allocated to userBalanceList");
            exit(1);
        }
        //wrap balance         
        strcpy(userFail, user_name);
        strcat(userFail, " does not exist in the group\r\n");            
        return userFail; 
    }
    if (prev_user == group->users) {
        // user could be first or second since previous is first
        if (strcmp(user_name, prev_user->name) == 0) {
            // this is the special case of first user
            int balance_len = snprintf(NULL, 0, "%.2f", prev_user->balance);
            char balance_str[balance_len + 1];
            snprintf(balance_str, balance_len + 1, "%.2f", prev_user->balance);
            if ((userB = malloc(balance_len + 10)) == NULL) {
                perror("Error resising the memory allocated to userBalanceList");
                exit(1);
            }
            //wrap balance 
            strcpy(userB, balance_str); 
            strcat(userB, "\r\n");
            return userB;
        }
    }
    //wrap balance    
    char *next_balance = "";
    sprintf(next_balance,"%.2f", prev_user->next->balance);
    if ((userB = malloc(strlen(next_balance) + 10)) == NULL) {
        perror("Error resising the memory allocated to userBalanceList");
        exit(1);
    }
    //wrap balance 
    strcpy(userB, next_balance);  
    strcat(userB, "\r\n");
    return userB;
}


/* Return a pointer to the user prior to the one in group with user_name. If 
* the matching user is the first in the list (i.e. there is no prior user in 
* the list), return a pointer to the matching user itself. If no matching user 
* exists, return NULL. 
*
* The reason for returning the prior user is that returning the matching user 
* itself does not allow us to change the user that occurs before the
* matching user, and some of the functions you will implement require that
* we be able to do this.
*/
User *find_prev_user(Group *group, const char *user_name) {
    User * current_user = group->users;
    // return NULL for no users in this group
    if (current_user == NULL) { 
        return NULL;
    }
    // special case where user we want is first
    if (strcmp(current_user->name, user_name) == 0) {
        return current_user;
    }
    while (current_user->next != NULL) {
        if (strcmp(current_user->next->name, user_name) == 0) {
            // we've found the user so return the previous one
            return current_user;
        }
    current_user = current_user->next;
    }
    // if we get this far without returning, current_user is last,
    // and we have already checked the last element
    return NULL;
}


/* Add the transaction represented by user_name and amount to the appropriate 
* transaction list, and update the balances of the corresponding user and group. 
* Note that updating a user's balance might require the user to be moved to a
* different position in the list to keep the list in sorted order. Returns 0 on
* success, and -1 if the specified user does not exist.
*/
char *add_xct(Group *group, const char *user_name, double amount) {
    User *this_user;
    User *prev = find_prev_user(group, user_name);
    char *userFail;
    char *userXCT;
    if (prev == NULL) {
        if ((userFail = malloc(strlen(user_name) + 35)) == NULL) {
            perror("Error resising the memory allocated to userBalanceList");
            exit(1);
        }
        strcpy(userFail, user_name);
        strcat(userFail, " does not exist in the group\r\n");            
        return userFail; 
    }
    // but find_prev_user gets the PREVIOUS user, so correct
    if (prev == group->users) {
        // user could be first or second since previous is first
        if (strcmp(user_name, prev->name) == 0) {
            // this is the special case of first user
            this_user = prev;
        } else {
            this_user = prev->next;
        }
    } else {
        this_user = prev->next;
    }


    Xct *newxct;
    if ((newxct = malloc(sizeof(Xct))) == NULL) {
        perror("Error allocating space for new Xct");
        exit(1);
    }
    // set the fields of the new transaction node
    // first allocate space for the name
    int needed_space = strlen(user_name) + 1;
    if ((newxct->name = malloc(needed_space)) == NULL) {
         perror("Error allocating space for new xct name");
         exit(1);
    }
    strncpy(newxct->name, user_name, needed_space);
    newxct->amount = amount;


    // insert this xct  at the front of the list
    newxct->next = group->xcts;
    group->xcts = newxct;


    // first readjust the balance
    this_user->balance = this_user->balance + amount;


    // since we are only ever increasing this user's balance they can only
    // go further towards the end of the linked list
    //   So keep shifting if the following user has a smaller balance


    while (this_user->next != NULL &&
                  this_user->balance > this_user->next->balance ) {
        // he remains as this user but the user next gets shifted
        // to be behind him
        if (prev == this_user) {
            User *shift = this_user->next;
            this_user->next = shift->next;
            prev = shift;
            prev->next = this_user;
            group->users = prev;
        } else { // ordinary case in the middle
            User *shift = this_user->next;
            prev->next = shift;
            this_user->next = shift->next;
            shift->next = this_user;
        }
    }
    if ((userXCT = malloc(25)) == NULL) {
            perror("Error resising the memory allocated to userBalanceList");
            exit(1);
    }
    
    strcpy(userXCT, "Balance updated\r\n");            
    return userXCT; 
}

lists.h source code

Code:

#ifndef LISTS_H
#define LISTS_H


struct group {
    char *name;
    struct user *users;
    struct xct *xcts;
    struct group *next;
};


struct user {
    char *name;
    double balance;
    struct user *next;
};


struct xct{
    char *name;
    double amount;
    struct xct *next;
};


typedef struct group Group;
typedef struct user User;
typedef struct xct Xct;


char *add_group(Group **group_list, const char *group_name, char *user_name);
char *list_groups(Group *group_list);
Group *find_group(Group *group_list, const char *group_name);


char *add_user(Group *group, const char *user_name);
char *list_users(Group *group);
char *user_balance(Group *group, const char *user_name);
User *find_prev_user(Group *group, const char *user_name);


char *add_xct(Group *group, const char *user_name, double amount);


#endif

Makefile source code

Code:

CC = gcc
CFLAGS = -Wall -Werror -g


buxfer: buxfer.o lists.o lists.h
    $(CC) $(CFLAGS) -o buxfer buxfer.o lists.o


buxfer.o: buxfer.c lists.h
    $(CC) $(CFLAGS) -c buxfer.c


lists.o: lists.c lists.h
    $(CC) $(CFLAGS) -c lists.c


clean: 
    rm buxfer *.o

Code:

    while (current != NULL) {
        if ((groupList = realloc(groupList, sizeof(groupList) + strlen(current->name) + 10)) == NULL) {
            perror("Error resising the memory allocated to groupList");
            exit(1);
        }
        strcat(groupList, current->name);
        strcat(groupList, "\r\n");
        current = current->next;
    }

You need to lookup the difference between sizeof() and strlen().
You're not making an array which is getting larger and larger with each string you append.

If fact, all your realloc's use sizeof() where they should be using strlen().

Code:

            if ((groupFail = malloc(strlen(group_name) + 30)) == NULL) {
        if ((userFail = malloc(strlen(user_name) + 30)) == NULL) {
    if ((userSucc = malloc(strlen(user_name) + 35)) == NULL) {
        if ((userFail = malloc(strlen(user_name) + 35)) == NULL) {
            if ((userB = malloc(balance_len + 10)) == NULL) {
    if ((userB = malloc(strlen(next_balance) + 10)) == NULL) {
        if ((userFail = malloc(strlen(user_name) + 35)) == NULL) {

Sorry, but this looks like the desperate act of someone trying to patch up multiple allocation failures by adding magic numbers to make the problem go away.
Except it only makes the problem "invisible" in your current limited tests. When people start running it with longer strings, larger text files etc, the problems will be back.

Sure, in a few places, you're preparing to add several strings at once, but the amount you allocate bears no relation to the lengths of the string constants you add.

If you're allocating space for strings, then the only constant you should be adding is +1 to account for the \0 at the end.
Anything else just looks like voodoo programming.

Rather than copy/pasting the code to manage allocated strings, use a couple of utility functions.

Code:

char *copyString ( const char *s ) {
  char *result = malloc( strlen(s) + 1 );
  if ( result ) {
    strcpy(result,s);
  }
  return result;
}
char *expandString ( char *current, const char *s ) {
  size_t newlen = strlen(current) + strlen(s) + 1;
  char *result = realloc( current, newlen );
  if ( result ) {
    strcat(result,s);
  }
  return result;
}

Yes, it is inefficient when appending lots of short strings onto the end of long strings, but you really need to focus on getting something working in the first instance.


So rather than making random length guesses

Code:

        if ((userFail = malloc(strlen(user_name) + 35)) == NULL) {
            perror("Error resising the memory allocated to userBalanceList");
            exit(1);
        }
        //wrap balance         
        strcpy(userFail, user_name);
        strcat(userFail, " does not exist in the group\r\n");            
        return userFail;

You would do

Code:

userFail = copyString(user_name);
userFail = expandString(userFail," does not exist in the group\r\n"); 
return userFail;